Skip to content

Take over PR #3888: Add unit tests for benchmarking metrics and document metric_R2 - #4041

Merged
dlebauer merged 22 commits into
PecanProject:developfrom
ayushman1210:pr_3888
Aug 20, 2026
Merged

Take over PR #3888: Add unit tests for benchmarking metrics and document metric_R2#4041
dlebauer merged 22 commits into
PecanProject:developfrom
ayushman1210:pr_3888

Conversation

@ayushman1210

@ayushman1210 ayushman1210 commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

I am taking over #3888 PR because the original author has been inactive, and I didn't have the permissions to push directly to their fork.
This PR includes all of @tanmaydimriGSOC's original commits, plus the requested fixes to unblock it

  • Removed the PEcAn.logger mock from the top of test-metrics.R.
  • Refactored metric_R2.R to remove the unreliable lm() fallback, now using stats::cor()^2.
  • Tightened the unit tests for metric_RMSE (expecting exactly 0) and metric_R2 (expecting NA and the standard stats::cor warning for constant inputs).

This can be merged to fully close out #3888

@ayushman1210
ayushman1210 requested a review from divine7022 June 30, 2026 15:47
@divine7022

divine7022 commented Jul 1, 2026

Copy link
Copy Markdown
Member

per yesterday's discussion, please try pushing to 3888, since you haven't actually tried
don't really want to have diverging PRs on same

divine7022
divine7022 requested changes Jul 13, 2026
View reviewed changes

@divine7022 divine7022 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall, and thanks for picking this up cleanly. tanmay's commits are preserved. dropped inline, few things before we commit
re run the devtools::document('modules/benchmark') to regenerate docs it is not in synce

Comment thread modules/benchmark/R/metric_R2.R Outdated
Comment thread modules/benchmark/R/metric_R2.R Outdated
Comment thread modules/benchmark/R/metric_R2.R Outdated
Comment thread modules/benchmark/tests/testthat/test-metrics.R Outdated
Comment thread modules/benchmark/R/metric_R2.R
@ayushman1210
ayushman1210 requested a review from divine7022 July 20, 2026 05:13
@divine7022

divine7022 commented Jul 21, 2026

Copy link
Copy Markdown
Member

ci is failing, please regenerate docs by running devtools::document() from package root and run Rscript scripts/generate_dependencies.R generate docker depends

@divine7022 divine7022 mentioned this pull request Jul 21, 2026
Merged
1 task
@dlebauer

dlebauer commented Jul 30, 2026

Copy link
Copy Markdown
Member

@ayushman1210 can you please

git checkout pr_3888 # the branch this PR is from
make document
git status
git add models/sipnet/man/read_restart.SIPNET.Rd
git commit -m "Regenerate SIPNET documentation"
git push

dlebauer
dlebauer approved these changes Jul 30, 2026
View reviewed changes

@dlebauer dlebauer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding tests! Ready to merge when CI passes (see prev. comment about regenerating docs)

@divine7022 divine7022 mentioned this pull request Jul 31, 2026
Closed
7 tasks
@ayushman1210 ayushman1210 mentioned this pull request Aug 4, 2026
Merged
11 tasks
@ayushman1210

ayushman1210 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

@dlebauer Commit f14de15b6 ("style: regenerate package documentation") has already regenerated the documentation

@dlebauer

dlebauer commented Aug 18, 2026

Copy link
Copy Markdown
Member

@ayushman1210 have you incorporated all comments from #3888? If so, can 3888 be closed?

@ayushman1210

ayushman1210 commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Yes @dlebauer I’ve incorporated all the comments from PR #3888.
You can close it now
thanks !!

@ayushman1210
ayushman1210 requested a review from dlebauer August 18, 2026 18:47
@dlebauer dlebauer mentioned this pull request Aug 18, 2026
Closed
divine7022
divine7022 approved these changes Aug 18, 2026
View reviewed changes

@divine7022 divine7022 left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!, looks good now. all CI checks are passing
good to merge

@dlebauer
dlebauer added this pull request to the merge queue Aug 20, 2026
Merged via the queue into PecanProject:develop with commit 9ef10b4 Aug 20, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@divine7022 divine7022 divine7022 approved these changes
@dlebauer dlebauer Awaiting requested review from dlebauer

Assignees

No one assigned

Labels

models modules tests

Projects

GSOC Benchmarking and Validation
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ayushman1210 @divine7022 @dlebauer @tanmaydimriGSOC